fix(batch-exports): replace invalid unicode with ? #21174
Conversation
| return orjson.dumps(d, default=str) | ||
| try: | ||
| return orjson.dumps(d, default=str) | ||
| except (UnicodeEncodeError, TypeError): |
There was a problem hiding this comment.
Strange that the exception raised doesn't match the docs:
If orjson.dumps() is given a str that does not contain valid UTF-8, orjson.JSONEncodeError is raised. If loads() receives invalid UTF-8, orjson.JSONDecodeError is raised.
However, I verified and it's indeed raising a TypeError (and obviously the unit test covers it). Wondering if we should be safe and catch orjson.JSONEncodeError just in case this is a bug that's fixed upstream at some point.
There was a problem hiding this comment.
Ok, I've figured it out: orjson.JSONEncodeError is an alias of TypeError (see: https://github.com/ijl/orjson/blob/master/src/typeref.rs#L197, and this issue). So, really, orjson is raising a TypeError.
There was a problem hiding this comment.
This means that this:
try:
return orjson.dumps(d, default=str)
except orjson.JSONEncodeError:
...Would also work (since orjson.JSONEncodeError is TypeError).
There was a problem hiding this comment.
One (nitty) thing though: UnicodeEncodeError is not the exception being raised, but the __cause__, so I would consider dropping that from the except and keeping just TypeError or orjson.JSONEncodeError.
|
thought: We really need to stop doing JSON unless user explicitly requests JSON in blob storage exports... |
bretthoerner
force-pushed
the
brett/batch-export-unicode
branch
from
19a9068 to
53a8418
Compare
Problem
orjsonis very strict about Unicode (but doesn't seem to provide a way to do error replacement on its own). Users have hit this in prod.Changes
Catch rare errors, do Unicode error replacement, then use
orjson.dumpsagain.👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
Yes.
How did you test this code?
Unit.